Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add 'Fail' mode when no shard selected #859

Open
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

nothrow
Copy link
Contributor

@nothrow nothrow commented Nov 11, 2024

Implementation for feature request & proposed solution in #858.

Adds new option for default_shard.

@magec
Copy link
Collaborator

magec commented Nov 12, 2024

I am fine with the change but I think there should be tests for this. We are already testing the default_shard parameter here, adding one test case for fail should be straight forward.

@lc-guy
Copy link

lc-guy commented Mar 25, 2025

@nothrow @magec I've implemented a test case for this:

diff --git a/tests/ruby/sharding_spec.rb b/tests/ruby/sharding_spec.rb
index 746627d..456997c 100644
--- a/tests/ruby/sharding_spec.rb
+++ b/tests/ruby/sharding_spec.rb
@@ -129,6 +129,26 @@ describe "Sharding" do
           end
         end
 
+        context "when no_shard_specified_behavior config is set to fail" do
+          let(:no_shard_specified_behavior) { "fail" }
+
+          context "with no shard comment" do
+            it "fails and doesn't send the query" do
+              conn = PG.connect(processes.pgcat.connection_string("sharded_db", "sharding_user"))
+              expect { conn.async_exec("SELECT 1 + 2").to_a }.to raise_error(PG::SystemError).with_message(/NoShardSelected/)
+            end
+          end
+
+          context "with a shard comment" do
+            it "honors the comment" do
+              conn = PG.connect(processes.pgcat.connection_string("sharded_db", "sharding_user"))
+              25.times { conn.async_exec("#{comment_to_use} SELECT 1 + 2") }
+
+              expect(processes.all_databases.map(&:count_select_1_plus_2)).to eq([0, 25, 0])
+            end
+          end
+        end
+
         context "when no_shard_specified_behavior config is set to random_healthy" do
           let(:no_shard_specified_behavior) { "random_healthy" }

Hopefully this can be merged soon :)

@magec
Copy link
Collaborator

magec commented Mar 25, 2025

I' fine with merging this. What do you think @drdrsh ?

@magec magec requested a review from drdrsh March 25, 2025 11:35
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants